SendPrimitiveAsync throws InvalidOperationException for enums #266
Conversation
Microsoft.Kiota.Http.HttpClientLibrary.Tests/RequestAdapterTests.cs
Outdated
Show resolved
Hide resolved
| @@ -293,7 +295,70 @@ public ISerializationWriterFactory SerializationWriterFactory | |||
| { | |||
| result = rootNode.GetDateValue(); | |||
| } | |||
| else throw new InvalidOperationException("error handling the response, unexpected type"); | |||
| else if( | |||
| Nullable.GetUnderlyingType(modelType) is { IsEnum: true } underlyingType && | |||
There was a problem hiding this comment.
We're going to need the AOT annotation on the generic type so people don't get errors when compiling with trimming.
https://github.com/microsoft/kiota-serialization-json-dotnet/blob/d0454daecb7ec2327f3e55d642fa510cf617e8b5/src/JsonParseNode.cs#L579
There was a problem hiding this comment.
The AOT attribute will also be required in the public method declaration I believe.
There was a problem hiding this comment.
Do you mean something like this?
public async Task<ModelType?> SendPrimitiveAsync<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicFields)] ModelType>(RequestInformation requestInfo, Dictionary<string, ParsableFactory<IParsable>>? errorMapping = default, CancellationToken cancellationToken = default)
There was a problem hiding this comment.
@andrueastman @baywet
It's probably a breaking change which requires changes in Microsoft.Kiota.Abstractions
error IL2095: 'DynamicallyAccessedMemberTypes' in 'DynamicallyAccessedMembersAttribute' on the generic parameter 'ModelType' of 'Microsoft.Kiota.Http.HttpClientLibrary.HttpClientRequestAdapter.SendPrimitiveAsync(RequestInformation, Dictionary<String, ParsableFactory>, CancellationToken)' don't match overridden generic parameter 'ModelType' of 'Microsoft.Kiota.Abstractions.IRequestAdapter.SendPrimitiveAsync(RequestInformation, Dictionary<String, ParsableFactory>, CancellationToken)'. All overridden members must have the same 'DynamicallyAccessedMembersAttribute' usage.
Is DynamicallyAccessedMembersAttribute necessary for SendPrimitiveAsync? Reflection is used on the underlying type, not directly on ModelType.
There was a problem hiding this comment.
I don't know the internals of AOT/trimming/the type system well enough to have a definitive answer answer to that question.
For changes impacting parse node and others, we did update the original interfaces, and that didn't result in a breaking change for anyone besides maybe people already doing AOT, but they were already broken.
I guess a first step here could be to look at the implementation/annotations on Nullable.GetUnderlyingType(modelType) and see what it requires (any dynamic attribute?) to know whether we need to propagate things here.
There was a problem hiding this comment.
At first sight, no dynamic attribute is used in the implementation
There was a problem hiding this comment.
I think if we move the functionality to the abstractions side, things will become clearer/different. So, let's make that change first...
|
Do you think that similar fix for SendPrimitiveCollectionAsync should go there or to https://github.com/microsoft/kiota-serialization-json-dotnet/blob/main/src/JsonParseNode.cs#L310 ? |
I think the similar fix should be implemented in the json parse node, but in that case, I think it'd be beneficial to move all the code getting the enum value in the abstractions so we don't duplicate it. Thoughts @andrueastman ? |
That's right. We will need a similar fix in the parsenode as well.
I agree with this too. Let's definitely do this if we can. |
|
So, what's the plan? |
|
I agree that a mono-repo would make things simpler here. However, I don't want you to be blocked because of that change, we don't have timelines for that change yet even though it's a high-priority item for us.
How does that work for you? |
I will try to update repos one by one. |
|
Thanks for the contribution @MartinM85. We'll close this one for now as we have moved the sources. |
Closes microsoft/kiota-dotnet#274